[project-redesign-help-dialog]mainリポジトリへのマージに向けた最終調整 #2160
Conversation
There was a problem hiding this comment.
いよいよですね！！！
ちょっと何点か、mainマージするにあたってご相談したいことが思いついたのでコメントしました 🙇
結構今まで気にしてなかったことをコメントしているかもなので、不明な点があれば何でもお聞きいただけると 🙇
ちなみに全然関係ないのですが、最近Storybookを導入してみました。
↓みたいな感じでstoriesというのを書けば、
https://github.com/VOICEVOX/voicevox/blob/1f39897f28ffaf93cd5df3e391a4c5a22e1d37cf/src/components/Dialog/UpdateNotificationDialog/index.stories.ts
↓こんな感じでコンポーネントや挙動をカタログみたいに出せたりします。
https://667d9c007418420dbb5b0f75-cpnbqzwmig.chromatic.com/?path=/story/components-dialog-updatenotificationdialog--opened
Vuexに依存してない形じゃないとまだStory書きにくいのですが、たぶんBase系とかはVuexに依存してないと思うので、もしご興味あれば･･･！
|
|
||
| .list-inner { | ||
| display: flex; | ||
| flex-direction: column; | ||
| padding: vars.$padding-2; | ||
| } | ||
|
|
||
| .list-label { | ||
| padding: 8px 16px; | ||
| padding-top: 16px; | ||
| color: colors.$display-sub; |
There was a problem hiding this comment.
あ！ vars.$paddingを使う・使わないのルールはどうしていきましょう？ 👀
見た感じほとんどのpaddingにvarsが使われてそう？
4pxのとこもあるけど･･･どうしたものか 😇
padding-halfを定義するとか･･･？あるいは例外的に4pxでも良いとか･･･。
あとremを使ってるところもありそうでした･･･！（BaseDocumentView.vueあたり）
揺れがちになってしまうので、メンテナンス面考えるとなんとなくでも良いので決めといちゃえばコーディングしやすいかなと･･･！
varsなら使って良いとか、4px/8px/16px/24pxならOKとか、0.25rem/0.5rem/1rem/1.5remもOKとか。基本remにしましょうとか！
とりあえずえいやで決めちゃって、後から流動的に変えていくのが良さそう･･･？
「vars/px/remどれでも良いけど、特に理由なければこの中から使ってくださいリスト」だけ用意しておいて、あとあと統一とか･･･？
あるいはもうremのが良い的な感じでしたら基本rem統一とか、おいおいスタイルガイドを作っていくとかでも良いと思います！
@romot-co
突然すみません！
そこそこ統一的なスタイルにしていくためにspacingとかを統一していきたいのですが、おすすめの統一方法とかあればご助言いただきたく･･･！ 👀
（ドキュメント書いておくとか･･･？とても大変そう･･･）
There was a problem hiding this comment.
@Hiroshiba @takusea
こちらありがとうございます！
MaterialDesignだとデザイントークンベースで管理が一般的かな…？と思うのですが、
- 今後はCSS Variablesを利用する(動的変更が可能 / MaterialDesignベースだとこちらが基本)
- 現状分はSCSS変数を利用し、必要になった段階でリファクタリング
- サイズは
remベースにし、ベースフォントサイズを16pxにする(計算しやすい・スケールが容易)
という形でどうでしょうか！
いずれにしても段階的に移行になると思うため、しばらくはある程度統一されていなくても気にせず、
一定程度固まってきた段階でルールを決める方がうまくいくかと思います！
(CSSは容易に壊れる＆例外が出まくるので、完璧を目指さないほうがよさそう…)
There was a problem hiding this comment.
@romot-co ありがとうございます！
確かに完璧を目指すのはちょっと違うかもですね！
個人的には、サイズはremベースに賛成です。border-sizeとか例外はちょくちょく許容しても良さそう！
CSS VariablesとSCSS変数どちらに寄せていくか難しいですね･･･。
SCSS変数だとコンパイル時に気づけるというのはありますが、結局CSS変数も要るので宣言が二度手間ですし･･･。
とりあえず現状はSCSS変数で、必要になった段階でファクタリング検討で賛成です！
@takusea さん的にはどうでしょう？ 👀
多分現状からだと「marginとpaddingがpx単位のとrem単位のがバラバラなので統一する、あとはmainマージする前にやるかマージ後にやるか」なのかなと思ってます！
（現状、BaseDocumentView.vueはrem単位、それ以外はpx単位になってる？）
CSS Variablesとかはまあ、もっと色々できてきた後においおいで･･･！
There was a problem hiding this comment.
一応、DocumentViewはドキュメントに対するスタイリングとして、他のUIに対するスタイリングとは別の扱いにしてました（marginはここでだけ使うようにしている、とか）。ただ、フォントの基底となるサイズを16pxにするのであればそれをベースに作るように合わせたほうがわかりやすそうです！
現状html要素に15px、body要素に14pxのfont-sizeが掛かっていてややこしい感じになっているので、その点でもremの数値を16pxにするのは賛成です！旧デザイン側への影響が懸念なのでを気をつけつつ…！
There was a problem hiding this comment.
現状html要素に15px、body要素に14pxのfont-sizeが掛かっていてややこしい感じになっているので
！！！！！！！！
本当だ、大変失礼しました！！！！
一旦DocumentViewはrem、ほかはpxで承知しました！！
将来的には合わせていきたい認識が揃ったので良かったということで･･･！！ 🙇
･････でもUIのfontが14pxなのはそれでもう慣れてしまったかもですね。。。。
pxベース側に揃えていくのも結構いいことなのかなとちょっと思いました。。。
src/styles/variables.scss
Outdated
| :root { | ||
| --size-basis: 8px; | ||
| --padding-basis: 8px; | ||
| --gap-basis: 8px; | ||
| --radius-basis: 8px; | ||
| } | ||
|
|
||
| $size-scrollbar: calc(var(--size-basis) * 1.5); | ||
| $size-icon: calc(var(--size-basis) * 2); | ||
| $size-indicator: calc(var(--size-basis) * 3); | ||
| $size-control: calc(var(--size-basis) * 4); | ||
| $size-listitem: calc(var(--size-basis) * 5); | ||
| $size-fab: calc(var(--size-basis) * 6); | ||
|
|
||
| $padding-1: var(--padding-basis); | ||
| $padding-2: calc(var(--padding-basis) * 2); | ||
|
|
||
| $gap-1: var(--gap-basis); | ||
| $gap-2: calc(var(--gap-basis) * 2); | ||
|
|
||
| $radius-1: var(--radius-basis); | ||
| $radius-2: calc(var(--radius-basis) * 2); | ||
|
|
||
| $transition-duration: 100ms; |
There was a problem hiding this comment.
もしかしたらこちらもv2に移動させたほうが良いかも･･･？ （迷ってます）
というのも、同じ名前の変数名を定義したくなったときに衝突しそうだな～というのと、v2用だとわかるようにしておくと他の方がわかりやすいかなと。
There was a problem hiding this comment.
複数v2向けのscssファイルを作るならstyles/下にv2フォルダを作ってmixin.scss含め新デザイン側で使うファイルをそこに纏めちゃっても良さそう？とも思いました
There was a problem hiding this comment.
確かにフォルダを分けてあげて、色々と混じらないようにする方が良さそう！！
|
@takusea あっすみません！！ そして今気づいたんですが、mainブランチにマージするプルリクエストは |
|
了解です！お手数おかけしました…！ |
|
@takusea ！！ 宛先変更で大丈夫そうです！（思いつかなかった） |
| <style scoped lang="scss"> | ||
| // detailsタグのスタイル | ||
| details { | ||
| summary { | ||
| display: list-item; | ||
| cursor: pointer; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
@sevenc-nanashi ちょっとした共有です！
ここのdetailsのスタイルちょっとこっちに移動させていただきました！
|
今の時点でのプルリクエストのレビューをして、問題なさそうなことを確認しました！ あとはこのディレクトリ分けをどうするかかなと･･･！ あ、よしなにタイトルを変えていただければ！ |
takusea
changed the title
|
PRタイトル変更しました！
見分けがつきやすいのとstyles/下が散らかりすぎないようにしたいので、フォルダ分けるように変更する方向でいこうと思います！ |
|
フォルダを分割する形に変更しました。 |
Hiroshiba
merged commit 2b1aec7
into
VOICEVOX:project-redisign-help-dialog
## 内容 project-redesign-help-dialogの内容をmainリポジトリへマージします。 ## 関連 Issue - VOICEVOX/voicevox_project#40 ## その他 - #2160
内容
project-redesign-help-dialogの内容をmainリポジトリへマージします。